-
Notifications
You must be signed in to change notification settings - Fork 844
Move PyWInObject_Free* methods behind PY_INTERFACE_POSTCALL #2467
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Prevent access violation if no GIL is aquired.
|
I pushed another commit with more occurences. |
| PyWinObject_FreeWCHAR(pszAssoc); | ||
| PY_INTERFACE_POSTCALL; | ||
| PyWinObject_FreeWCHAR(pszAssoc); | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And I don't know why clang-format has a problem.
You have some trailing whitespaces. You can run either of these commands to fix it locally:
clang-format -i $(git ls-files '*.cpp' ':!:com/win32comext/mapi/src/MAPIStubLibrary/')clang-format -i com/win32comext/shell/src/PyIQueryAssociations.cpppre-commit run clang-format -a
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the hint. I ran pre-commit. And added the fix to PyObject_CleanupDEFCONTEXTMENU as well.
|
I'm surprised that none of the CI tests does catch any of these. For issue #2415 the reason is that the CI pipeline does not run the COM tests. What about the issue fixed with PR #2466 ? |
|
|
||
| PyComTypeObject PyIQueryAssociations::type("PyIQueryAssociations", &PyIUnknown::type, sizeof(PyIQueryAssociations), | ||
| PyIQueryAssociations_methods, GET_PYCOM_CTOR(PyIQueryAssociations)); | ||
| // This file implements the IQueryAssociations Interface and Gateway for Python. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you may have accidentally changed line endings or something in this file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, downloading the raw File shows CRLF instead of LF of all the other files. I will try to fix it.
|
Sorry, I was quiet busy the last weeks. I will take a Look into it. thank you @Avasam for the hint. |
I think CI does run the COM tests via pywin32_testall.py. The problem is likely to be that CI simply has a very limited set of COM objects available, which probably does not include the shell - whereas running them locally typically does. If we believe COM tests aren't being run at all, then that should certainly be fixed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me, thanks!
|
I'm also concerned by the lack of CI failures on Python3.12 given the claims here and in #2466. But that's something that can be looked into separately, see if it can be improved, and tracked in a new issue. I'm myself not fully aware what does or doesn't run on CI. I try to run relevant demos when I think about it (as shown by the shear amount of demos I've been fixing 😆). Maybe we'd need some contributor guidelines on that. We have more and more static checkers, which helps prevent certain class of issues on the Python side, but won't replace dynamic tests. |
As follow-up to #2466.
Those changes have have the same pattern.
In addition, I'm not sure aboutshell.cppmethodPyObject_CleanupDEFCONTEXTMENU. It has two relevant calls, but I did not include any change to this one.Please note, I just searched and edited all those methods. This pull request is not tested in some way. ;-)
Edit: Fixed
PyObject_CleanupDEFCONTEXTMENUas well.